Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace meta_tags.csv with hparams.yaml #1271

Merged

Conversation

S-aiueo32
Copy link
Contributor

@S-aiueo32 S-aiueo32 commented Mar 28, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1212 .

@pep8speaks
Copy link

pep8speaks commented Mar 28, 2020

Hello @S-aiueo32! Thanks for updating this PR.

Line 20:111: E501 line too long (117 > 110 characters)
Line 1575:111: E501 line too long (117 > 110 characters)

Line 15:111: E501 line too long (118 > 110 characters)

Comment last updated at 2020-05-13 11:10:28 UTC

Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool. maybe a test for saving to and loading from a yaml file would be good.

pytorch_lightning/core/saving.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/tensorboard.py Show resolved Hide resolved
@Borda Borda added the feature Is an improvement or enhancement label Mar 29, 2020
requirements.txt Outdated
@@ -5,4 +5,4 @@ numpy>=1.16.4
torch>=1.1
tensorboard>=1.14
future>=0.17.1 # required for builtins in setup.py

pyyaml>=5.3.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it really the minimal version we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the current latest version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we put there like 3.x or lover since we are using quite general features?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@S-aiueo32 Don't forget to also add it to the environment.yml file

@mergify
Copy link
Contributor

mergify bot commented Mar 30, 2020

This pull request is now in conflict... :(

@mergify mergify bot requested a review from a team March 30, 2020 22:33
@Borda Borda added this to the 0.7.2 milestone Apr 2, 2020
@Borda
Copy link
Member

Borda commented Apr 2, 2020

@S-aiueo32 how is it going? it would be great to have it in next week release 0.7.2

@S-aiueo32
Copy link
Contributor Author

@Borda I'm sorry, but I'm too busy for about two weeks this week to be able to respond. I recognize that the major changes are complete and all that remains is testing and minor revisions. If you don't mind, could you let the core contributor take over the work?

@Borda Borda self-assigned this Apr 2, 2020
@williamFalcon
Copy link
Contributor

@Borda mind taking over this one?

@williamFalcon williamFalcon modified the milestones: 0.7.2, 0.7.3 Apr 3, 2020
@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2020

This pull request is now in conflict... :(

@Borda
Copy link
Member

Borda commented Apr 8, 2020

changelog need to be rebased on new release #1419

@Borda
Copy link
Member

Borda commented Apr 11, 2020

@S-aiueo32 may you pls allow editing your PR, I have prepared to rebase, but don't have permission to push it... 🤖

@S-aiueo32
Copy link
Contributor Author

S-aiueo32 commented Apr 12, 2020

@Borda confirmed?

@S-aiueo32 S-aiueo32 force-pushed the feature/replace-mata-tags-with-yaml branch from bc64951 to 5d27e34 Compare May 13, 2020 11:05
@S-aiueo32
Copy link
Contributor Author

@Borda @awaelchli @williamFalcon All checks pass, thanks!

@Borda
Copy link
Member

Borda commented May 13, 2020

it looks good to me, I have pinged @PyTorchLightning/core-contributors to get this done

Copy link
Member

@justusschock justusschock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Borda Borda requested a review from jeremyjordan May 13, 2020 13:04
@Borda Borda merged commit 22d7d03 into Lightning-AI:master May 13, 2020
@Borda
Copy link
Member

Borda commented May 13, 2020

@S-aiueo32 Great work! and thx for your patience 🦝

Copy link
Contributor

@jeremyjordan jeremyjordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we include a test for tests/test_deprecated.py?

@Borda
Copy link
Member

Borda commented May 13, 2020

shall we include a test for tests/test_deprecated.py?

completely forgot... @S-aiueo32 mind make followup or with deprecation tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for loading flattened meta_tags.csv
8 participants